Add search button to z.ex.search plugin #46
Conversation
This commit adds functionality to the z.ex.search plugin: - New toolbar button with PDF icon in the main Eclipse toolbar - Button executes ~/git/content/_scripts/buildRCPScript.sh when pressed - BuildRCPScriptHandler class handles script execution in background thread - Shows success/error dialog with script output - Uses Eclipse 3.x API (org.eclipse.ui.menus) for toolbar contribution Note: The hard-coded script path (~/git/content/_scripts/buildRCPScript.sh) is intentional and points to a specific location in the user's home directory. Files added: - z.ex.search/src/z/ex/search/BuildRCPScriptHandler.java - z.ex.search/icons/pdf.svg Files modified: - z.ex.search/plugin.xml (added command and toolbar contribution) - z.ex.search/build.properties (included icons directory)
Summary of ChangesHello @vogella, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully adds a new toolbar button to execute a build script. The implementation correctly uses a background thread to prevent UI freezes. However, there are several opportunities for improvement to align with Eclipse development best practices. My review includes suggestions to use the Eclipse Jobs API instead of a raw thread for better IDE integration, improve path construction for platform independence, clean up logging and imports, and enhance UI feedback for script failures. Applying these changes will increase the robustness and maintainability of the new functionality.
| Thread scriptThread = new Thread(() -> { | ||
| try { | ||
| ProcessBuilder pb = new ProcessBuilder(SCRIPT_PATH); | ||
| pb.directory(scriptFile.getParentFile()); | ||
| pb.redirectErrorStream(true); | ||
|
|
||
| Process process = pb.start(); | ||
|
|
||
| // Read and display output | ||
| StringBuilder output = new StringBuilder(); | ||
| try (BufferedReader reader = new BufferedReader( | ||
| new InputStreamReader(process.getInputStream()))) { | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| output.append(line).append("\n"); | ||
| System.out.println(line); | ||
| } | ||
| } | ||
|
|
||
| int exitCode = process.waitFor(); | ||
|
|
||
| final String message; | ||
| if (exitCode == 0) { | ||
| message = "RCP build script executed successfully."; | ||
| } else { | ||
| message = "RCP build script failed with exit code: " + exitCode + | ||
| "\n\nOutput:\n" + output.toString(); | ||
| } | ||
|
|
||
| Display.getDefault().asyncExec(() -> { | ||
| MessageDialog.openInformation( | ||
| Display.getDefault().getActiveShell(), | ||
| "Build RCP Script", | ||
| message | ||
| ); | ||
| }); | ||
|
|
||
| } catch (IOException e) { | ||
| showError("Error executing script: " + e.getMessage()); | ||
| } catch (InterruptedException e) { | ||
| showError("Script execution interrupted: " + e.getMessage()); | ||
| Thread.currentThread().interrupt(); | ||
| } | ||
| }); | ||
|
|
||
| scriptThread.setDaemon(true); | ||
| scriptThread.start(); | ||
|
|
There was a problem hiding this comment.
While using a separate Thread prevents blocking the UI, the recommended approach in Eclipse plugin development is to use the Jobs API (org.eclipse.core.runtime.jobs.Job). Jobs provide better integration with the Eclipse UI, including progress reporting in the Progress view, cancellation support, and proper scheduling. Refactoring this to a Job would make the implementation more robust and provide a better user experience.
Here is a basic structure for converting this to a Job:
Job job = new Job("Building RCP") {
@Override
protected IStatus run(IProgressMonitor monitor) {
// Your script execution logic here...
// You can report progress via the monitor.
// Show dialogs using Display.getDefault().asyncExec(...)
// Return Status.OK_STATUS for success, or an error status.
return Status.OK_STATUS;
}
};
job.setUser(true); // Shows the job in the UI
job.schedule();| import org.eclipse.core.runtime.IStatus; | ||
| import org.eclipse.core.runtime.Status; | ||
| import org.eclipse.jface.dialogs.MessageDialog; | ||
| import org.eclipse.swt.widgets.Display; | ||
| import org.eclipse.ui.handlers.HandlerUtil; |
There was a problem hiding this comment.
The imports org.eclipse.core.runtime.IStatus, org.eclipse.core.runtime.Status, and org.eclipse.ui.handlers.HandlerUtil are not used and should be removed to keep the code clean.
| import org.eclipse.core.runtime.IStatus; | |
| import org.eclipse.core.runtime.Status; | |
| import org.eclipse.jface.dialogs.MessageDialog; | |
| import org.eclipse.swt.widgets.Display; | |
| import org.eclipse.ui.handlers.HandlerUtil; | |
| import org.eclipse.jface.dialogs.MessageDialog; | |
| import org.eclipse.swt.widgets.Display; |
| private static final String SCRIPT_PATH = System.getProperty("user.home") + | ||
| "/git/content/_scripts/buildRCPScript.sh"; |
There was a problem hiding this comment.
Using + to concatenate path components with a hardcoded separator / is not platform-independent. It's better to use java.nio.file.Paths.get() to construct the path. This makes the code cleaner and more robust across different operating systems. You will need to add import java.nio.file.Paths; for this change.
private static final String SCRIPT_PATH = java.nio.file.Paths.get(System.getProperty("user.home"), "git", "content", "_scripts", "buildRCPScript.sh").toString();| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| output.append(line).append("\n"); | ||
| System.out.println(line); |
There was a problem hiding this comment.
Printing script output directly to System.out can clutter the console and is generally not recommended for production code. For proper logging, consider using the Eclipse logging framework. Since the output is already collected and shown in the dialog on failure, this line seems redundant and can be removed.
| final String message; | ||
| if (exitCode == 0) { | ||
| message = "RCP build script executed successfully."; | ||
| } else { | ||
| message = "RCP build script failed with exit code: " + exitCode + | ||
| "\n\nOutput:\n" + output.toString(); | ||
| } | ||
|
|
||
| Display.getDefault().asyncExec(() -> { | ||
| MessageDialog.openInformation( | ||
| Display.getDefault().getActiveShell(), | ||
| "Build RCP Script", | ||
| message | ||
| ); | ||
| }); |
There was a problem hiding this comment.
When the script execution fails, an information dialog (MessageDialog.openInformation) is shown. It would be more appropriate and consistent with the showError method to use an error dialog (MessageDialog.openError) to clearly signal to the user that something went wrong.
final boolean success = exitCode == 0;
Display.getDefault().asyncExec(() -> {
if (success) {
MessageDialog.openInformation(
Display.getDefault().getActiveShell(),
"Build RCP Script",
"RCP build script executed successfully."
);
} else {
String message = "RCP build script failed with exit code: " + exitCode +
"\n\nOutput:\n" + output.toString();
MessageDialog.openError(
Display.getDefault().getActiveShell(),
"Build RCP Script Error",
message
);
}
});This commit addresses code review feedback and implements Eclipse best practices: 1. Replace raw Thread with Eclipse Jobs API (org.eclipse.core.runtime.jobs.Job) - Better integration with Eclipse UI progress reporting - Shows job in Progress view with 'Building RCP' label - Support for cancellation via IProgressMonitor - Proper status reporting (OK, ERROR, CANCEL) 2. Use Paths.get() for platform-independent path construction - Replaced string concatenation with java.nio.file.Paths - More robust across different operating systems 3. Remove unused imports - Removed org.eclipse.ui.handlers.HandlerUtil (not used) 4. Remove console logging - Removed System.out.println() calls (redundant) - Output is already collected and shown in dialogs 5. Use appropriate dialog types - Success: MessageDialog.openInformation - Failure: MessageDialog.openError (changed from openInformation) - More consistent with Eclipse UI conventions These improvements enhance robustness, maintainability, and user experience by following Eclipse plugin development best practices.
When the RCP build script executes successfully, the user is now prompted with a question dialog asking if they want to open the generated PDF file. Changes: - Success dialog changed from openInformation to openQuestion - Adds prompt: "Do you want to open the PDF file?" - New openPdfFile() method to handle PDF opening - Uses SWT Program.launch() to open PDF with system default application - PDF path hard-coded to ~/git/content/output.pdf (intentional) - Validates PDF file exists before attempting to open - Shows error if PDF not found or no PDF viewer associated User can now: 1. Click "Open" to view the PDF in their system's default PDF viewer 2. Click "Cancel" to dismiss the dialog without opening the file Note: Both the script path and PDF output path are intentionally hard-coded to specific locations in the user's home directory.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new toolbar button to execute a build script. The implementation correctly uses an Eclipse Job to run the script in the background, preventing UI freezes. The code is generally well-structured. I have a few suggestions to improve flexibility, correctness, and user experience. My main points are about making the hardcoded script paths configurable, ensuring the background process is terminated reliably on cancellation, and avoiding redundant error notifications to the user.
| private static final String SCRIPT_PATH = Paths.get(System.getProperty("user.home"), | ||
| "git", "content", "_scripts", "buildRCPScript.sh").toString(); | ||
| private static final String PDF_OUTPUT_PATH = Paths.get(System.getProperty("user.home"), | ||
| "git", "content", "output.pdf").toString(); |
There was a problem hiding this comment.
The script path and PDF output path are hardcoded. While the PR description mentions this is intentional, it severely limits the plugin's flexibility and makes it difficult for other users to adapt it to their environment. A better approach would be to make these paths configurable via the Eclipse preference store (IPreferenceStore). This would allow users to set their own paths in the plugin's preference page without needing to modify the source code.
| while ((line = reader.readLine()) != null) { | ||
| output.append(line).append("\n"); | ||
| if (monitor.isCanceled()) { | ||
| process.destroy(); |
There was a problem hiding this comment.
The process.destroy() method sends a standard termination signal, which the process can choose to ignore. When a user cancels a job, they usually expect it to stop immediately. Using process.destroyForcibly() is more reliable for ensuring the process is terminated upon cancellation.
| process.destroy(); | |
| process.destroyForcibly(); |
| return Status.OK_STATUS; | ||
|
|
||
| } catch (IOException e) { | ||
| showError("Error executing script: " + e.getMessage()); |
There was a problem hiding this comment.
Calling showError() here in addition to returning an error IStatus can lead to duplicate error notifications for the user (one from your dialog, another from the Eclipse Jobs UI). It's better to rely on the returned IStatus to report the error. The message from the status will be shown to the user in the Progress view.
| showError("Error executing script: " + e.getMessage()); | |
| // showError("Error executing script: " + e.getMessage()); |
| return new Status(IStatus.ERROR, "z.ex.search", | ||
| "Error executing script", e); | ||
| } catch (InterruptedException e) { | ||
| showError("Script execution interrupted: " + e.getMessage()); |
There was a problem hiding this comment.
Similar to the IOException handler, calling showError() here is redundant because Status.CANCEL_STATUS will notify the user of the cancellation via the Eclipse Jobs UI. Removing this call will prevent duplicate notifications.
| showError("Script execution interrupted: " + e.getMessage()); | |
| // showError("Script execution interrupted: " + e.getMessage()); |
This commit addresses code review feedback to improve flexibility and robustness:
1. Make paths configurable via Eclipse preferences (MAJOR IMPROVEMENT)
- Created PreferenceConstants class with default paths
- Created PreferenceInitializer for default preference values
- Created BuildScriptPreferencePage for user configuration
- Registered preference page and initializer in plugin.xml
- Users can now customize script and PDF paths via:
Window > Preferences > Build Script
- Defaults to original hard-coded paths for backward compatibility
2. Improve process termination on cancellation
- Changed process.destroy() to process.destroyForcibly()
- Ensures immediate termination when user cancels the job
- More reliable than standard destroy() which can be ignored
3. Remove redundant error notifications
- Removed showError() calls in IOException and InterruptedException handlers
- IStatus errors are automatically shown in Eclipse Jobs UI
- Prevents duplicate error dialogs for better UX
4. Code improvements
- Removed unused java.nio.file.Paths import (no longer needed)
- Read paths from preference store in execute() and openPdfFile()
- Maintain backward compatibility with existing default paths
Benefits:
- Plugin is now flexible and reusable for different environments
- Better user control without modifying source code
- More reliable cancellation handling
- Cleaner error reporting without duplication
- Follows Eclipse plugin development best practices
Files added:
- z.ex.search/src/z/ex/search/PreferenceConstants.java
- z.ex.search/src/z/ex/search/PreferenceInitializer.java
- z.ex.search/src/z/ex/search/BuildScriptPreferencePage.java
Files modified:
- z.ex.search/src/z/ex/search/BuildRCPScriptHandler.java
- z.ex.search/plugin.xml
This commit replaces all MessageDialog popups with the modern Eclipse Notification API for a better user experience. Changes: 1. Success notification with interactive "Open PDF" button - NotificationPopup appears in bottom-right corner - User can click "Open PDF" button directly in notification - Non-blocking, allows continued work while notification is visible 2. Error notifications for build failures - Shows error message in notification popup - Truncates long error messages (>300 chars) for readability - Consistent with modern Eclipse UI patterns 3. Updated MANIFEST.MF - Added org.eclipse.jface dependency for NotificationPopup API 4. Improved user experience - Non-modal notifications don't block workflow - Notifications auto-dismiss after timeout - Action buttons directly in notification - Modern, consistent with Eclipse platform Implementation: - Uses NotificationPopup.forDisplay() fluent API - WidgetFactory for creating notification content - GridLayout for organizing notification widgets - onSelect() handlers for interactive buttons Benefits: - Less intrusive than modal dialogs - Users can continue working while notification is visible - Better integration with Eclipse UI conventions - More professional, modern appearance Files modified: - z.ex.search/src/z/ex/search/BuildRCPScriptHandler.java - z.ex.search/META-INF/MANIFEST.MF
The previous commit used NotificationPopup API which is not available in the target platform, causing build failures. Changes: - Reverted from NotificationPopup to MessageDialog - Removed org.eclipse.jface dependency from MANIFEST.MF - Renamed methods to showSuccessDialog() and showErrorDialog() - Removed unnecessary imports (WidgetFactory, SWT, Composite) - Kept all code review improvements: * Configurable preferences ✓ * process.destroyForcibly() for cancellation ✓ * No duplicate error notifications ✓ * Eclipse Jobs API ✓ User experience remains the same: - Success: Question dialog with "Open" and "Cancel" buttons - Failure: Error dialog with exit code and output - All paths configurable via preferences This resolves the build failure while maintaining all improvements.
vogella
deleted the
claude/add-search-plugin-button-019B21ZEGm95jTUDexTzQXDG
branch
This commit adds functionality to the z.ex.search plugin:
Note: The hard-coded script path (~/git/content/_scripts/buildRCPScript.sh) is intentional and points to a specific location in the user's home directory.
Files added:
Files modified: